Raise ValueError in _get_ordered_vertices() with "mixed" order#595
Raise ValueError in _get_ordered_vertices() with "mixed" order#595aulemahal merged 4 commits intoxarray-contrib:mainfrom
_get_ordered_vertices() with "mixed" order#595Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #595 +/- ##
==========================================
+ Coverage 85.78% 86.95% +1.17%
==========================================
Files 13 15 +2
Lines 2364 3159 +795
Branches 183 302 +119
==========================================
+ Hits 2028 2747 +719
- Misses 303 367 +64
- Partials 33 45 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cf_xarray/helpers.py
Outdated
| elif order == "descending": | ||
| endpoints = np.maximum(bounds[..., :, 0], bounds[..., :, 1]) | ||
| last_endpoint = np.minimum(bounds[..., -1, 0], bounds[..., -1, 1]) | ||
| elif order == "mixed": |
There was a problem hiding this comment.
Even if this is a private function, I would avoid having code that can result in an UnboundLocalError.
I suggest either replacing it with a bare else, or adding something like:
else:
raise NotImplementedError(f"{order = }")There was a problem hiding this comment.
The new code raises a ValueError with order=mixed, so it shouldn't result in UnboundLocalError. However, I updated this conditional to else as a fall-back for any order value (even if it should only be ascending, descending, or mixed).
tomvothecoder
force-pushed
the
bug/594-mixed-coords
branch
from
9c287cf to
1a5eb22
Compare
for more information, see https://pre-commit.ci
| else: | ||
| raise NotImplementedError( | ||
| f"Cannot determine vertices for non-monotonic bounds with {order} core " |
There was a problem hiding this comment.
|
@malmans2 Just pinging to let you know this is ready for re-review. Thanks! |
|
Looks good to me! |
|
Thank you @malmans2 for the review and @aulemahal for merging! |
* origin/main: Add HEALPix grid mapping support to GridMapping (#614) [pre-commit.ci] pre-commit autoupdate (#612) Raise ValueError in `_get_ordered_vertices()` with "mixed" order (#595) Bump actions/upload-artifact from 4 to 5 (#605) Bump actions/checkout from 5 to 6 (#606) Dont preserve attributes when creating bounds (#608) Bump actions/download-artifact from 5 to 6 (#603) Bump astral-sh/setup-uv from 4 to 7 (#604) Bump codecov/codecov-action from 5.5.0 to 5.5.1 (#599) Bump actions/setup-python from 5 to 6 (#597) [pre-commit.ci] pre-commit autoupdate (#600) Fix singleton coord in core dim orders (#592) # Conflicts: # cf_xarray/accessor.py
3 participants
A user reported an issue in #594 where
_get_ordered_verticesfails to set theendpointsvariable when the order of core dims are"mixed". In their case, the longitude coordinates are circular (0–360), and the 1-D core dimension isn’t strictly monotonic because it crosses the seam at 0°.We should raise a
ValueErrorfor"mixed"cases, rather than silently fail (or normalize).Long-term solution
Longer term, somebody can open a PR to consider opt-in support for circular axes, e.g.
bounds_to_vertices(..., circular_period=360.0, start=None). Withcircular_periodset, cf-xarray could detect circular monotonicity, rotate away from the seam, and proceed safely.